Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Q Chitter Challenge #2178

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

SomthingInteresting
Copy link

No description provided.

Copy link

@umutbaykan umutbaykan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry about the grammar and tone, we were in a rush! :)

Comment on lines 9 to 14
new_peep = Peep.new(
id: peep['id'].to_i,
content: peep['content'],
user_id: peep['user_id'].to_i,
created_at: peep['created_at']
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_peep = Peep.new
new_peep.id = peep['id'].to_i..
etc. etc.


new_peep
end
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return result before the def all method finishes (ends)

# Debug statement
# puts "Peep created_at: #{new_peep.created_at.inspect}"

new_peep

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be removed

spec/integration/chitter_app_spec.rb Outdated Show resolved Hide resolved
"Hello, world!"
]

actual_peeps = response.body.scan(/<p class="peep__content">(.+)<\/p>/).flatten

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need additional tests to check for format of peeps, as in they include the username, time etc..


expected_username = "orangeman"
actual_username = response.body.scan(/<p class="peep__username">(.+)<\/p>/).flatten.first

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can add one more test to check full format of one peep?

Comment on lines +4 to +8
def reset_tables
seed_sql = File.read('spec/seeds/chitter_seed.sql')
connection = PG.connect({ host: '127.0.0.1', dbname: 'chitter_test' })
connection.exec(seed_sql)
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same method happens in user repo tests, can we move those two to a singular location to have control from a single point?

email: user['email']
)
end
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to see return result to clarify what it is returning

Comment on lines +1 to +2
TRUNCATE TABLE users, peeps, notifications RESTART IDENTITY;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this file, but would be good to see the create table sql file in here as well

require 'users'
require 'users_repository'

def reset_tables

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants